Add DataSource/FileSource proto hooks and FileScanConfig serde - #23683
Add DataSource/FileSource proto hooks and FileScanConfig serde#23683kumarUjjawal wants to merge 5 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #23683 +/- ##
==========================================
+ Coverage 80.75% 80.82% +0.06%
==========================================
Files 1096 1098 +2
Lines 373588 374459 +871
Branches 373588 374459 +871
==========================================
+ Hits 301687 302651 +964
+ Misses 53898 53752 -146
- Partials 18003 18056 +53 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@adriangb could you take a look whenever you get the time. |
|
Instead of duplicating could we move the code as you've done but then rewrite serialize_file_scan_config/parse_protobuf_file_scan_config as delegating wrappers to the new inline methods? then we don't need to test byte equality, we avoid drift, etc. |
| Ok(FileGroup::new(files)) | ||
| } | ||
|
|
||
| fn partitioned_file_to_proto(pf: &PartitionedFile) -> Result<protobuf::PartitionedFile> { |
| ); | ||
| } | ||
|
|
||
| mod file_scan_config_serde { |
There was a problem hiding this comment.
can we un-indent this mod and add another #[cfg(test)] on top?
d66a690 to
6345b35
Compare
buraksenn
left a comment
There was a problem hiding this comment.
LGTM. Thanks for the work, I'll start working on sub issues today
Thanks for the review. |
|
@adriangb can you take a look. |
adriangb
left a comment
There was a problem hiding this comment.
Can you take a look at the comments about moving TryFromProto and the implementations for PartitionedFile and FileGroup? Ideally we could do that as precursor PRs.
| Ok(FileGroup::new(files)) | ||
| } | ||
|
|
||
| pub(crate) fn partitioned_file_to_proto( |
There was a problem hiding this comment.
It seems like this could be just fn (nothing outside of this module uses it)
| }) | ||
| } | ||
|
|
||
| pub(crate) fn partitioned_file_from_proto( |
There was a problem hiding this comment.
It seems like this could be just fn (nothing outside of this module uses it)
| /// Each concrete [`FileSource::try_to_proto`] | ||
| /// wraps the returned value in its own `*ScanExecNode`. Byte-compatible with | ||
| /// the former `serialize_file_scan_config` in `datafusion-proto`. | ||
| pub fn to_proto_conf( |
There was a problem hiding this comment.
Should we stick to the naming we're using elsewhere try_to_proto, try_from_proto?
| /// table schema via [`FileScanConfig::parse_table_schema_from_proto`]). | ||
| /// | ||
| /// Byte-compatible with the former `parse_protobuf_file_scan_config`. | ||
| pub fn from_proto_conf( |
| FileScanConfig::parse_table_schema_from_proto(proto) | ||
| } | ||
|
|
||
| pub fn parse_protobuf_file_scan_config( |
There was a problem hiding this comment.
Presumably will be deprecated in future PR?
|
|
||
| pub(crate) fn partitioned_file_to_proto( | ||
| pf: &PartitionedFile, | ||
| ) -> Result<protobuf::PartitionedFile> { |
There was a problem hiding this comment.
Instead of a free-standing method, which duplicates:
datafusion/datafusion/proto/src/physical_plan/to_proto.rs
Lines 427 to 459 in 0b07e58
I think if we move TryFromProto into datafusion-proto-models then we can depend on it here and just move the impl for these types and avoid duplicating it.
At the very least I think a better shape would be to
impl PartitionedFile {
fn try_to_proto(...) { }
fn try_from_proto(...) {}
}6345b35 to
a6d0291
Compare
…on-datasource (apache#24006) ## Which issue does this PR close? - Part of apache#23494. Precursor for apache#23497 / apache#23683 (`DataSource` / `FileSource` proto hooks) and for apache#23752. ## Rationale for this change The protobuf conversions for the file-scan leaf types — `PartitionedFile`, `FileGroup`, `FileRange` — live in `datafusion-proto` as `TryFromProto` impls, because that is historically the only crate that can name both sides (the DataFusion type and the prost message are both foreign to it, hence the `TryFromProto` workaround trait in the first place). That placement means any *other* crate that needs those conversions has to reimplement them. apache#23683 hits exactly this: a `FileSource` serializing its own scan config needs to encode file groups, so the first cut of that PR grew a private second copy of the `PartitionedFile` wire logic inside `datafusion-datasource`, which can then drift from the central serializer. The same will be true of every source migrated under apache#23516–apache#23518. Nothing about these conversions needs `datafusion-proto`: they are plain data, with `ScalarValue` / `Statistics` / `Schema` going through `datafusion-proto-common`. They belong next to the types. ## What changes are included in this PR? - New `datafusion_datasource::proto` module, behind a new `proto` feature on `datafusion-datasource` (off by default; `datafusion-proto` enables it): - `FileRange::try_to_proto` / `try_from_proto` - `PartitionedFile::try_to_proto` / `try_from_proto` - `FileGroup` <-> `protobuf::FileGroup` - `datafusion-proto`'s `TryFromProto` impls for those types become one-line shims delegating to the new impls, so every existing caller keeps working and the two sides cannot disagree. ### Why these are `TryFrom` and not `try_to_proto` hooks `TryFromProto` exists because `datafusion-proto` owns neither side of the conversions it hosts: with both the DataFusion type and the prost message foreign to it, `impl TryFrom<protobuf::X> for X` is rejected by the orphan rule, so a local trait was the only way to say the same thing. Moving a conversion into the crate that owns the DataFusion type removes that constraint, and `&T` is `#[fundamental]`, so both directions are expressible with the standard trait (checked, not assumed): ```rust impl TryFrom<&protobuf::PartitionedFile> for PartitionedFile // ok impl TryFrom<&PartitionedFile> for protobuf::PartitionedFile // ok impl TryFrom<&[PartitionedFile]> for protobuf::FileGroup // E0117 ``` The last one is why `protobuf::FileGroup`'s *slice* conversion stays a `TryFromProto` shim: `&[PartitionedFile]` is not a type this crate owns, while `&FileGroup` is. Callers inside DataFusion go through `FileGroup`. So the rule this PR sets for the rest of apache#23494: **plain data uses `TryFrom`; anything needing an encode/decode context keeps the `try_to_proto(ctx)` / `try_from_proto(node, ctx)` hooks**, because the standard trait cannot carry that second argument. Usefully, none of the ~40 `TryFromProto`/`FromProto` impls needs a context, and nothing that needs one was ever a `TryFromProto` impl — the two categories are already disjoint, so the shape now tells a reader whether a conversion recurses. ### Why now `FromProto` / `TryFromProto` were added in apache#21929, *after* the 54.0.0 release, and 54.1.0 was cut before any of this landed — so they have never shipped in a release. Replacing them with the standard traits, and eventually deleting them, is a no-op for semver **today** and a major breaking change the moment 55.0.0 goes out. The same applies to the six inherent `try_to_proto` / `try_from_proto` methods this PR would otherwise have added: they are new, unreleased API, so choosing their final shape costs nothing right now. The other reason to settle it here rather than in a follow-up: this is the PR that establishes the pattern for the data-source family (apache#23516-apache#23519 and apache#23752 / apache#23781 are all queued behind it). Whichever shape merges first is the one they will copy. Retiring the remaining ~34 impls is still its own follow-up. Two notes for whoever picks it up: the sink and format-option conversions can move next to their types the same way, but the ones for `datafusion-common`-owned types (`JoinType`, `NullEquality`, `TableReference`, `UnnestOptions`, ...) cannot — `datafusion-common` cannot depend on `datafusion-proto-models` (it is underneath it via `datafusion-proto-common`). Their legal home is `proto-models` itself, implementing on the local proto type, which is already how `proto-common` hosts the `ScalarValue` / `Statistics` conversions. ## Are these changes tested? Yes. - New unit tests in `datafusion_datasource::proto` covering the `PartitionedFile` round trip (path, size, mtime, partition values, range, arrow schema, statistics), the `FileGroup` round trip, and the invalid-path error. - The existing `datafusion-proto` tests now exercise the delegating shims, so they also pin the shims themselves. - `datafusion-proto`, all features: 227 passed / 0 failed. - `datafusion-datasource` with `proto`: 180 passed / 0 failed. - Full workspace run: 10347 passed (`cargo test --profile ci --workspace --lib --tests --features avro,json,backtrace,extended_tests,recursive_protection,parquet_encryption`). The only failures are 8 backtrace-symbolization tests in `datafusion-common`, a crate this PR does not touch and which sits below every crate it does; they fail the same way on the base commit on macOS. - `cargo fmt` and `ci/scripts/rust_clippy.sh` clean. ## Are there any user-facing changes? The protobuf wire format is unchanged, and no existing API changes shape. Additive: - New `proto` feature on `datafusion-datasource` (off by default). - New `TryFrom` impls in both directions between `FileRange`, `PartitionedFile`, `FileGroup` and their protobuf messages, under that feature. No new names are added to the crate's API surface: the trait is `core::convert::TryFrom`. Note for reviewers: while writing the round-trip test I found that `PartitionedFile` statistics do not round-trip cleanly on `main` — filed as apache#23998. This PR preserves that behavior exactly rather than changing decode semantics in a refactor; the test documents it. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Which issue does this PR close?
Rationale for this change
File scan serialization currently depends on central type downcasts in
datafusion-proto.This PR adds the foundation needed to move each data source to its own protobuf hooks while preserving the existing wire format and fallback behavior.
It unblocks the concrete source migrations tracked by #23516, #23517, and #23518.
What changes are included in this PR?
try_to_protohooks toDataSourceandFileSource.DataSourceExecthroughFileScanConfigto its concreteFileSource.FileScanConfigprotobuf encoding and decoding in the datasource crate.Ok(None).Are these changes tested?
Yes
Are there any user-facing changes?
This adds non-breaking, feature-gated public serialization hooks for data source and file source implementations.